Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for restic --ignore-inode #75

Closed
wants to merge 2 commits into from

Conversation

weisdd
Copy link

@weisdd weisdd commented Dec 21, 2020

Issue
When backing up data, restic considers the following attributes to decide whether files have changed since the last snapshot:

  • Type (file, symlink, or directory?)
  • Modification time
  • Size
  • Inode number (internal number used to reference a file in a file system)

The last one, inode number, should not be considered in case of FUSE-based filesystems (e.g. mounted via s3-fuse like minio) as they do not support inode consistency, which basically means that inode numbers change on every remount. Otherwise, we'll get two side effects:

  • since restic thinks files are new, it has to process them all over again
    => might slow down the process;
  • the restic statistics gets distorted
    e.g. "files_new":109,"files_changed":6202,"files_unmodified":525 - here, in my test, actually, 109 files were added, but none of 6202 really changed, and 525 happened to have the same inode numbers - just pure luck)
    => metrics are also distorted as a consequence.

Note: fortunately, all of that doesn't lead to anything bad like data corruption, it's just some extra work and incorrect statistics.

Proposed change
We only need to add an option to ignore inode changes (--ignore-inode).

Important notes
I'm not an expert in golang nor in your package, so the way I implemented the feature might not be the best one, and maybe there are extra cases where the --ignore-inode flag should be called.
I tested the change with v0.10.0-alpha.0 for backing up Minio via s3-fuse, and I'm pretty sure it will work with any other version of the package as there's nothing version specific.

As for the stash plugins, something simple like this will bring the support wherever it's needed:
cmd.Flags().BoolVar(&opt.setupOptions.IgnoreInode, "ignore-inode", opt.setupOptions.IgnoreInode, "Specify whether restic should ignore inodes (useful for FUSE mounts)")

@weisdd
Copy link
Author

weisdd commented Dec 21, 2020

stashed/stash#1138

@hossainemruz hossainemruz self-requested a review December 21, 2020 09:56
@hossainemruz
Copy link
Contributor

Hi @weisdd Thank you for the PR. Please sign your commit. You can follow the steps recommended by DCO bot: https://github.com/stashed/apimachinery/pull/75/checks?check_run_id=1588113497

Btw, since --ignore-inode is a property of backend, it should be a configurable field in Repository CRD. It can also go in runtimeSettings of BackupConfiguration similar to nice and ionice. I will talk with our team about where it should fit. It may take some time as we are middle of some other tasks. Please be patient. This definitely going to be part of our next major release.

@hossainemruz hossainemruz added the bug Something isn't working label Dec 21, 2020
Signed-off-by: Igor Beliakov <[email protected]>
@weisdd
Copy link
Author

weisdd commented Dec 21, 2020

Hi @hossainemruz
I've just signed the commit, the DCO check is successful now.

Thanks for the extra details!
I chose the current implementation just because it lets me enable the flag dynamically through a helm chart with a Function.
Should the change go somewhere else, I'll still be happy to see that the PR served as a simple POC. :)

@hossainemruz
Copy link
Contributor

Should the change go somewhere else.

No. These changes are fine. You will also need to add this here: https://github.com/weisdd/apimachinery/blob/e7025ffbf8c9029da77c75c2e209ce9bad018baa/pkg/restic/commands.go#L165

We have to decide how should we resolve the value of w.config.IgnoreInode. It can come from a field of Repository CRD or from runtimeSettings.

@weisdd
Copy link
Author

weisdd commented Dec 21, 2020

@hossainemruz I intentionally skipped the function for stdin. My train of thought was that inode becomes an issue only when you mount an external filesystem in userspace and backup files from there. When it's something read from stdin, there should not be an intermediate inode to care about. Plus, the mtime will change anyway, so other factors will trigger restic to analyze the file. Thus, --ignore-inode should not make any difference in this context.

@hossainemruz
Copy link
Contributor

hossainemruz commented Dec 21, 2020

You are right @weisdd. We don't need --ignore-inode for backup stdin. Thank you for pointing it out.

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2021

CLA assistant check
All committers have signed the CLA.

@weisdd
Copy link
Author

weisdd commented May 6, 2021

Any plans to merge the PR or implement the functionality differently?

@hossainemruz
Copy link
Contributor

We are really sorry that it is taking too long. I am currently occupied with a different task. I will return back to Stash in a week or two. Then, I can revisit this PR.

@weisdd
Copy link
Author

weisdd commented Aug 21, 2021

@hossainemruz Honestly, I feel like closing the PR as it's been already 8 months for a simple flag.

@hossainemruz
Copy link
Contributor

Ahh. I forgot about this PR. Sorry, for the delay. I will take a look soon.

@hossainemruz
Copy link
Contributor

@weisdd We have decided to introduce a generic option to pass user arguments to the restic backup/restore command. Check this PR: #121

Now, you can pass any flags (including --ignore-inode) to the restic backup/restore command. Please see the example here: stashed/stash#1384

@weisdd
Copy link
Author

weisdd commented Sep 29, 2021

@hossainemruz that's even better than my solution, thanks!

@hossainemruz
Copy link
Contributor

Sorry, for this feature request took too long. We were busy with other stuff. Closing this PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants